Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DEPRECATION] Remove references to deprecated rand.Seed (#2650) #2950

Closed
wants to merge 2 commits into from

Conversation

lhardt
Copy link
Contributor

@lhardt lhardt commented Sep 27, 2024

Remove references to deprecated rand.Seed (#2650).

Context:

  • From Go 1.20 (Feb 2023) onwards, math/rand.Seed is deprecated, and math/rand is already seeded by default.
  • From Go 1.24 (Feb 2025?) onwards, math/rand.Seed will be a no-op, breaking the test TestPwgenNoCrand.

See method documentation in the link above for further info.

Code Changes:

  • We must have our own Rand variable, so that its value can be injected in tests;
  • I changed the test name of TestPwgenNoCrand to TestPwgenNoCrandFallback, to make the expected behaviour clearer;
  • Shred does not need to seed, nor does TestLoadFromEnv in config_test,
    • But note that TestPwgenNoCrandFallback must restore the randomness so that TestLoadFromEnv can work properly.

This also removes the side-effect that fsutil's Shred was overwriting the seed set by pwgen.

"fmt"
"io"
mrand "math/rand"
Copy link
Contributor Author

@lhardt lhardt Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This choice was questioned on orangekame's PR.

Howver, this aligns pwgen_test.go with the style found in rand.go, of the same package.

Moreover, it only affects 3 references in a single test

AnomalRoil
AnomalRoil previously approved these changes Sep 27, 2024
Copy link
Member

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!
If you can just lint it and accept our DCO and signoff your commit that'd be great.

dominikschulz
dominikschulz previously approved these changes Sep 28, 2024
@lhardt
Copy link
Contributor Author

lhardt commented Sep 29, 2024

Hi! Done the linting and signoff fixes. Since this is so close to October, would you mind if I reopen the PR on Tuesday?

@dominikschulz
Copy link
Member

@lhardt No problem at all. See you in #hacktober :)

@lhardt
Copy link
Contributor Author

lhardt commented Sep 30, 2024

Will be reopening it then. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants